Add BaseLanguageHandling option to Datagen #4606

sffc · 2024-02-13T23:28:56Z

I implemented the new option as BaseLanguageHandling::Retain or BaseLanguageHandling::Strip.

sffc · 2024-02-14T00:22:41Z

provider/datagen/src/driver.rs

@@ -370,23 +385,32 @@ impl DatagenDriver {
                        .iter()
                        .try_for_each(|(locale, (payload, _duration))| {
                            let mut iter = fallbacker_with_config.fallback_for(locale.clone());
+                            let mut first = true;


The effect of first is that it prevents non-base locales such as tlh-001 from being retained. I'm not sure if this is the behavior that we desire; it only comes up when an unsupported locale is requested and that locale is not a base language. I lean toward removing the check on first, which would cause tlh-001 to be retained.

I think it's fine to remove first, but we should probably print a warning alerting the user that the requested locale is not supported; if that's not the current behaviour. I think that should be enough indication that using the provider as is could return incorrect results.

Yep. Today I'm thinking that we probably should not include either tlh-001 or arc since there is no CLDR data for them, and therefore they are not really "supported". I'll work on that tweak.

robertbastian · 2024-02-14T10:16:21Z

provider/datagen/src/driver.rs

+    /// Sets the base language handling in runtime fallback mode.
+    ///
+    /// For more details, see [`BaseLanguageHandling`].
+    pub fn with_base_language_handling(self, base_language_handling: BaseLanguageHandling) -> Self {


issue: this option is only valid for a specific fallback mode. I'd prefer modelling it with a new fallback mode

I think it applies for both Runtime and RuntimeManual. It also applies for PreferredForExporter in certain cases; an user who wants to enable resolvedLocale queries but doesn't want to pick a specific fallback type. This means we would need to add 3 more fallback variants + any other variant that is added in the future that is similar to Runtime.

Good point about PreferredForExporter. With that option, the exporter can state its preferred fallback mode, but that question is completely separate from whether to include the base languages.

An exporter does not state its preferred fallback mode, it states supports_built_in_fallback. Datagen translates this to an actual mode, which we are free to change: true can be RuntimeThin, and false HybridThin, as you are currently making this the default already.

robertbastian · 2024-02-14T10:17:05Z

provider/datagen/src/lib.rs

+    /// By default, locales that inherit directly from "und" will be retained even if their data
+    /// is equal to "und". To override this behavior, see [`BaseLanguageHandling`].
+    ///


issue: I'm not convinced this is a good default. This increases baked data size, but supported locale queries are not a universal use case.

Happy to discuss the default. Given that it's needed for 402 and we've seen enough examples of use cases where people may want to do these queries out of the box, and the data size increase is fairly small (no regional variants), I lean toward including these by default. It's easy for power users to get the extra 5% data size benefit if they know what they are doing.

slightly lean towards the default of Strip. I think either way we will need careful documentation of supported locale queries and how they can work. We should do that documentation and clearly document the defaults.

provider/datagen/src/lib.rs

sffc · 2024-02-22T17:27:09Z

Current:

Runtime
RuntimeManual
Hybrid
Preresolved

Proposed to add:

RuntimeWithBaseLanguages
RuntimeManualWithBaseLanguages

@sffc - How to we turn off regional variants?
@robertbastian - Maybe something like en-XX to not include regional variants, this woulud be more granular because you could do de,en-ZZ for all de variants and no en variants
@sffc - I think it should be a variant, because it can also apply to regional variants like en-001. So you may want en-001-nochilds, which includes en and en-001 but not en-GB.

@robertbastian - Maybe we want something like

Deduplicated / Runtime2 {
  use_internal_fallback: bool,
  include_base_languages: bool,
}

Now:

#[non_exhaustive]
pub enum FallbackMode {
    #[default]
    DefaultForProvider,
    // same as Deduplicated { RuntimeFallbackLocation::Internal, BaseLanguageHandling::Strip }
    Runtime,
    // same as Deduplicated { RuntimeFallbackLocation::External, BaseLanguageHandling::Strip }
    RuntimeManual,
    Hybrid,
    Preresolved,
    Deduplicated(Deduplicated),
}

2.0:

#[non_exhaustive]
pub enum LocaleExpansionMode {
    Deduplicated(Deduplicated),
    Exhaustive(Exhaustive),
    Preresolved(Preresolved),
}

#[non_exhaustive]
pub enum RuntimeFallbackLocation {
    Internal,
    External,
}

#[non_exhaustive]
pub enum BaseLanguageHandling {
    #[default]
    Retain,
    Strip,
}

#[non_exhaustive]
#[derive(Default)]
pub struct Deduplicated {
  runtime_fallback_location: Option<RuntimeFallbackLocation>,
  base_language_handling: BaseLanguageHandling,
}

// resolve this against the exporter like
foo.runtime_fallback_location.unwrap_or_else(|| 
    if exporter.supports_runtime_fallback() {
        RuntimeFallbackLocation::Internal
    } else { 
        RuntimeFallbackLocation::External 
    }
)

#[non_exhaustive]
pub struct Exhaustive {}

#[non_exhaustive]
pub struct Preresolved {}

LGTM: @Manishearth @sffc @robertbastian

sffc · 2024-02-23T02:49:12Z

@robertbastian What do you want the CLI to look like in 1.5 and in 2.0?

robertbastian · 2024-02-23T08:14:29Z

I guess for now we flatten the enum like runtime-manual-retain and runtime-strip?

sffc · 2024-02-23T21:30:16Z

If we are going to end up with a base_language_handling option in the Deduplicated enum, then maybe it makes sense that it should be its own flag on the CLI.

robertbastian · 2024-02-23T22:27:09Z

It doesn't, because then someone will set base_language_handling for hybrid or preresolved and that doesn't make sense.

sffc · 2024-02-23T22:44:49Z

We have a lot of flags that are only used if some other flag is set. This would be the same.

sffc · 2024-02-24T02:00:48Z

@robertbastian I would like to merge this as-is because:

The final 2.0 CLI option should indeed be named --base-language-handling because that is how we handle all other cases where flags are not used depending on the value of other flags. If you really want I could make --base-language-handling return an error if set with the wrong value for --fallback.
In the API, we are changing it anyway in 2.0 as discussed above. The change proposed is the smallest change to the API. I don't want to implement only half of the 2.0 changes now. I would rather we, in another PR, implement LocaleExpansionMode as designed above. We could even do that in 1.5 and deprecate FallbackMode. In that PR, we could delete any unreleased APIs such as this one that are covered by LocaleExpansionMode.

jedel1043 · 2024-02-24T02:15:56Z

With my engine maintainer hat on, I have to agree with @sffc. Boa will require all engine embedders to use this feature, so from a docs perspective it's easier to guide our users to activate a simple flag than to explain to them which fallback variants are supported and how to activate them.

robertbastian

I still consider this additional flag tech debt.

Manishearth · 2024-02-26T17:40:19Z

provider/datagen/src/bin/datagen/args.rs

+    #[arg(long, value_enum, default_value_t = BaseLanguageHandling::Auto)]
+    #[arg(
+        help = "--fallback=runtime[-manual] only: Whether to retain or strip base languages. \
+                For details, read the Rust docs for BaseLanguageHandling."


nit: let's call the local one BaseLanguageHandlingArg, to avoid confusion when looking at the code. The one above has no docs.

Alternatively give it docs saying to look at the type in the config module.

Manishearth · 2024-02-26T17:42:26Z

provider/datagen/src/lib.rs

+    /// By default, locales that inherit directly from "und" will be retained even if their data
+    /// is equal to "und". To override this behavior, see [`BaseLanguageHandling`].
+    ///


slightly lean towards the default of Strip. I think either way we will need careful documentation of supported locale queries and how they can work. We should do that documentation and clearly document the defaults.

robertbastian

There's a lot of discussion going on in #4629 which I'd like to resolve before merging anything.

sffc · 2024-04-10T07:53:25Z

Need to re-do this on top of #4710

jedel1043 · 2024-04-25T01:00:21Z

Probably super seeded by #4836.

sffc · 2024-04-25T01:16:42Z

Probably super seeded by #4836.

Things this PR does that #4836 didn't do:

Added to CHANGELOG.md
Added "tlh-001" to the options test (to test an unsupported non-base locale)
Made retaining base languages the default option

sffc added 4 commits February 13, 2024 15:09

Add BaseLanguageHandling option to datagen

9727f40

Add test to demonstrate !first behavior

7ddf547

cargo make bakeddata

b278741

Add metrics to docs

e970c1a

sffc marked this pull request as ready for review February 14, 2024 00:20

sffc requested review from robertbastian, Manishearth and a team as code owners February 14, 2024 00:20

sffc requested review from jedel1043 and removed request for a team and Manishearth February 14, 2024 00:20

sffc commented Feb 14, 2024

View reviewed changes

sffc mentioned this pull request Feb 14, 2024

Draft sketch of ResolvedLocaleAdapter #4607

Draft

Add changelog

4f3670d

robertbastian reviewed Feb 14, 2024

View reviewed changes

sffc added the discuss-priority Discuss at the next ICU4X meeting label Feb 14, 2024

sffc removed the discuss-priority Discuss at the next ICU4X meeting label Feb 22, 2024

Docs

2b2400a

sffc requested a review from robertbastian February 24, 2024 01:56

sffc mentioned this pull request Feb 24, 2024

Implement Datagen API LocaleExpansionMode #4629

Closed

robertbastian approved these changes Feb 26, 2024

View reviewed changes

Manishearth reviewed Feb 26, 2024

View reviewed changes

robertbastian requested changes Feb 28, 2024

View reviewed changes

sffc added the discuss Discuss at a future ICU4X-SC meeting label Mar 7, 2024

sffc marked this pull request as draft April 10, 2024 07:53

robertbastian assigned sffc and robertbastian and unassigned sffc Jun 27, 2024

robertbastian added this to the ICU4X 2.0 ⟨P1⟩ milestone Jun 27, 2024

robertbastian removed the discuss Discuss at a future ICU4X-SC meeting label Jun 27, 2024

sffc removed this from the ICU4X 2.0 ⟨P1⟩ milestone Sep 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add BaseLanguageHandling option to Datagen #4606

Add BaseLanguageHandling option to Datagen #4606

sffc commented Feb 13, 2024 •

edited

Loading

sffc Feb 14, 2024

jedel1043 Feb 14, 2024 •

edited

Loading

sffc Feb 15, 2024

robertbastian Feb 14, 2024

jedel1043 Feb 14, 2024 •

edited

Loading

sffc Feb 15, 2024

robertbastian Feb 19, 2024

robertbastian Feb 14, 2024

sffc Feb 15, 2024

Manishearth Feb 26, 2024

sffc commented Feb 22, 2024

sffc commented Feb 23, 2024

robertbastian commented Feb 23, 2024 •

edited

Loading

sffc commented Feb 23, 2024

robertbastian commented Feb 23, 2024

sffc commented Feb 23, 2024

sffc commented Feb 24, 2024

jedel1043 commented Feb 24, 2024

robertbastian left a comment

Manishearth Feb 26, 2024

Manishearth Feb 26, 2024

robertbastian left a comment

sffc commented Apr 10, 2024

jedel1043 commented Apr 25, 2024

sffc commented Apr 25, 2024 •

edited

Loading

Add BaseLanguageHandling option to Datagen #4606

Are you sure you want to change the base?

Add BaseLanguageHandling option to Datagen #4606

Conversation

sffc commented Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

jedel1043 Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedel1043 Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc commented Feb 22, 2024

sffc commented Feb 23, 2024

robertbastian commented Feb 23, 2024 • edited Loading

sffc commented Feb 23, 2024

robertbastian commented Feb 23, 2024

sffc commented Feb 23, 2024

sffc commented Feb 24, 2024

jedel1043 commented Feb 24, 2024

robertbastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertbastian left a comment

Choose a reason for hiding this comment

sffc commented Apr 10, 2024

jedel1043 commented Apr 25, 2024

sffc commented Apr 25, 2024 • edited Loading

sffc commented Feb 13, 2024 •

edited

Loading

jedel1043 Feb 14, 2024 •

edited

Loading

jedel1043 Feb 14, 2024 •

edited

Loading

robertbastian commented Feb 23, 2024 •

edited

Loading

sffc commented Apr 25, 2024 •

edited

Loading